Size Change: +1.61 kB (0%)

Total Size: 1.71 MB

_{compressed-size-action}

Flaky tests detected in 06fdbf7.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/8009112699
📝 Reported issues:

• [Flaky Test] default to the only existing classic menu if there are no block menus #48033 in /test/e2e/specs/editor/blocks/navigation.spec.js

The release beta cycle might not be a good time to introduce a new public SlotFill.

We can use a private SlotFill (see example #49819) or add a component directly to the toolbar like BlockParentSelector. What do you think?

The release beta cycle might not be a good time to introduce a new public SlotFill.

We can use a private SlotFill (see example #49819) or add a component directly to the toolbar like BlockParentSelector. What do you think?

This is the expected change in the toolbar that triggered refactoring to introduce a new slot:

Add a "bound" icon in the block toolbar.

@SaxonF or @jasmussen, what would be the alternative to change the block's icon instead in a way that retains the original but also gives a clear indicator that it's connected? This way, we wouldn't have to introduce another toolbar group. In its current shape, should it even be a toolbar item? Is it actionable?

Anyway, the other aspect is that we should stop using the filters to integrate Block Bindings with the block editor, but instead consider it part of the framework that should be tightly integrated. In effect, the changes should go directly to the block toolbar to add icon indicator there. The border for the block's content can be integrated withing useBlockProps, etc.

This PR will need a slight refactor once #58895 is merged. Once that's done, we'll have proper mechanism to add logic to "bound" blocks.

We will need to cherry-pick this PR to the wp/6.5 branch, so I would wait to merge the #58895 refactoring until WP 6.5 RC1 so this bug fix lands without conflicts.

what would be the alternative to change the block's icon instead in a way that retains the original but also gives a clear indicator that it's connected? This way, we wouldn't have to introduce another toolbar group. In its current shape, should it even be a toolbar item? Is it actionable?

Functionally this seems okay in the name of shipping the work. That being said, it feels like there's potential for a deeper design iteration going forward, since the feature is so potent, so this is something we'll want to follow up on. The issue here goes back to the same issue we have elsewhere in the site editor, more clearly delineating what content is reusable/global, and what content is local. We haven't fully threaded the needle on how to address that; the purple sync color is a useful start, the "Content" panel and some of the work around pattern overrides can potentially be leveraged here as well. But ideally we can find one concept that would work across all of those, and make it much clearer for anyone editing, where and what they are changing.

Thanks for the feedback folks! I've moved the implementation directly to the <BlockToolbar>. My only concern with this approach is performance because then we have to run an additional selector for every block.

I've tried to use the private SlotFills but I wasn't quite able to make it work. Unlike the BlockControls SloFill, I saw duplicated elements (there was a button on the toolbar for each connected block). Perhaps they'd have to be deduplicated manually.

Wondering if we could highlight properties in the toolbar that are connected much like we will do in inspector where we use an icon. In toolbar we could highlight with colour + shape. If a block's "primary" attribute is connected we'd highlight the block icon e.g. a paragraph content.

So for an image that has src, link and alt text connected when in a template it would look something like

Any important information need a visual indicaotr but also needs to be provided in a semantic and accessible way. I'm not sure a button that doesn't do anything and its only used for visual purposes is ideal.

• The new ToolbarButton doesn't do anything.
• Toolbar are supposed to contain controls for actual actions. Toolbars aren't the right place for descriptions.
• The label of the ToolbarButton is too long as @Mamaduka reported. More importantly, labels and tooltips of buttons are meant to expose the button name, which is what the button does. Besides the fact this button doesn;;t do anything the text Connected to a block bindins source or even a horter version of it is a description of one of the block properties, not really the anme of the button action.
• Color alone can't be the only indication of the connected state.
• Consistency: the first button within the toolbar is supposed to:
  • Indicate the block type
  • Provide the transform to menu, which is communciated via the button description.
• Please, no purple or blue 'dots' to indicate a state: we already tried them in teh templates list and they're not great for accessibility. Also, toolbar buttons aren't the right place to indicate properties that belong to the block.

Overall, I'm not sure the toolbar is the right place to add an indicator to. I'd see the Settings panel a sa more appropriate palce, as that's where block properties and settings are available to users.

Interesting concept @SaxonF, thanks! That does look better visually IMO.

My only concern is that we would have to use custom fields-specific logic to handle this. Sources other than custom fields might not have fields whose names we can display here.

What if it just says "Connected to core/post-meta" ?

Interesting concept @SaxonF, thanks! That does look better visually IMO.

My only concern is that we would have to use custom fields-specific logic to handle this. Sources other than custom fields might not have fields whose names we can display here.

What if it just says "Connected to `core/post-meta`" ?

Actually, scratch that because each of block's attributes can be connected to multiple sources so that won't work either 🙁 .

What we'd need is a way to indicate the "Connected to " in the UI per attribute

Hi @afercia

I understand some of your concerns.

About the "button": We could get rid of the <ToolbarButton> and just use a <div> with an icon in it. What would you propose to use instead?

Toolbar are supposed to contain controls for actual actions. Toolbars aren't the right place for descriptions.

That's a subjective opinion about what the UI should look like so it's hard to argue one way or the other 🙂

The label of the ToolbarButton is too long

I've changed it to just "Connected" in bbd3fcc

Color alone can't be the only indication of the connected state.

I'm not sure I understand - It's not the only indicator, it's in addition to the indicators that are already present:

• block editing is disabled
• there's a icon in the toolbar

So, it's an improvement over the status quo.

Consistency: the first button within the toolbar is supposed to:

• Indicate the block type
• Provide the transform to menu, which is communciated via the button description.

That's again a subjective opinion about what UI should look like 🙂 Besides, e.g. the Button block has a parent selector as the first item in the toolbar.

Please, no purple or blue 'dots' to indicate a state: we already tried them in teh templates list and they're not great for accessibility. Also, toolbar buttons aren't the right place to indicate properties that belong to the block.

As of right now, there is no indication of the state so I don't understand how adding dots would be worse? The design proposed by @SaxonF also adds indicators of "connected" state in the block Settings as you suggest. Is that what you're proposing?

RE: Avoiding rendering a <button>. I'm not 100% sure how to proceed here. Here's what I tried:

1. Keep <ToolbarButton> but find a way to pass the as prop to render a different component like <ToolbarButton as={"div"}>. The <ToolbarButton> does not support the as prop.

2. Use the more generic ToolbarItem component. This is the current approach I tried in 06fdbf7. However, using the ToolbarItem also comes with a disadvantage of not having a label and tooltip as a button does.

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: michalczaplinski <[email protected]>
Co-authored-by: artemiomorales <[email protected]>
Co-authored-by: Mamaduka <[email protected]>
Co-authored-by: gziolo <[email protected]>
Co-authored-by: SantosGuillamot <[email protected]>
Co-authored-by: jasmussen <[email protected]>
Co-authored-by: SaxonF <[email protected]>
Co-authored-by: afercia <[email protected]>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

During my testing I've seen that it is adding the visual indicators when I bind a block that is not supported. Is this the expected behavior?

That's a good catch @SantosGuillamot . In addition to checking isConnected we should also check if the block can be bound using canBindBlock()

That's a good catch @SantosGuillamot . In addition to checking isConnected we should also check if the block can be bound using canBindBlock()

@michalczaplinski Addressed this in ac72c0a

I'm pretty happy with the PR now. If there aren't any more concerns I will merge later today :)

Not sure why "some checks were not successful" because looking at the individual checks they all seem to pass. I will merge now as this seems like a github reporting issue.

There's one last thing I've noticed, the block icon color in the List View is barely visible if the block is selected:

We can fix this in a quick follow-up PR.

It seems that when I click on a block that is connected it throws an error in the console:

Warning: Invalid DOM property `fill-rule`. Did you mean `fillRule`?

@michalczaplinski @artemiomorales Could you check if you are able to reproduce it and, in that case, fix it please?

Could you check if you are able to reproduce it and, in that case, fix it please?

@SantosGuillamot Fixed in the following commit on the same PR to fix the fill color.

I just cherry-picked this PR to the update/packages-6.5-rc1 branch to get it included in the next release